Skip to content

feat(multi-entry): add preserveModules option - #1237

Merged
shellscape merged 12 commits into
rollup:masterfrom
814k31:support-preserve-modules
Oct 24, 2022
Merged

feat(multi-entry): add preserveModules option#1237
shellscape merged 12 commits into
rollup:masterfrom
814k31:support-preserve-modules

Conversation

@814k31

@814k31 814k31 commented Aug 9, 2022

Copy link
Copy Markdown
Contributor

Rollup Plugin Name: @rollup/plugin-multi-entry

This PR contains:
Probably somewhere in the grey area of a bugfix and a feature

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

Its "kind of" a breaking change? Breaking in the fact that people who use this plugin with preserveModules=true will experience a change by upgrading, however the change will be probably an outcome that they would expect from this plugin when using preverveModules=true

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

Description

When preserveModules is set on the output, its expected that the filenames and structure will not be altered.
This library is always setting the files names to be multi-entry.js and if there are multiple it is multi-entry1.js and so on.

My desired out come is that I can use the multi-entry plugin to target all src files within my project to be built so when using the library I can easily do
import { ExampleComponent } from 'example-library/nested-directory/exampleComponent';

when the src file structure is:
example-library -> src -> nested-directory -> exampleComponent.js

…e = true

BREAKING CHANGES: Changes the default behaviour to not alter filenames
if preserveModules=true
@814k31
814k31 requested a review from shellscape as a code owner August 9, 2022 14:52
@814k31

814k31 commented Aug 9, 2022

Copy link
Copy Markdown
Contributor Author

Because I dont have pnp on my machine, I haven't actually been able to run the tests unfortunately... just fyi but that should at least describe my expectation

Edit: Tests have been run now

Comment thread packages/multi-entry/src/index.js Outdated
@814k31 814k31 changed the title Update functionality to maintain file output names when preserveModul… @rollup/plugin-multi-entry - Maintain file output names when output.preserveModules = true Aug 9, 2022
@814k31

814k31 commented Aug 9, 2022

Copy link
Copy Markdown
Contributor Author

I am open to other suggestions on how to solve this, one being allowing config.entryFileName to be set to false or null to fallback to the options which would go into making this a non breaking change. However I wanted to try provide a solution that doesnt bloat configuration options

@814k31

814k31 commented Aug 20, 2022

Copy link
Copy Markdown
Contributor Author

@shellscape not to be a pain but could I bug you to have a look at this
It's actually stopping my enterprise from adopting rollup in general

I'm very flexible on the implementation here

Really I just need a method to make sure the plugin doesn't always use the default entry filename so that I can use rollups default behaviour with preserveModules and this plugin together

@shellscape

Copy link
Copy Markdown
Collaborator

I generally don't have much stake in this plugin. Please check out the history and which maintainers have contributed to it in the past for their feedback.

@814k31

814k31 commented Aug 20, 2022

Copy link
Copy Markdown
Contributor Author

@shellscape will do thanks for replying so quickly

@814k31

814k31 commented Aug 26, 2022

Copy link
Copy Markdown
Contributor Author

Hello @guybedford @NotWoods

sorry to drag you guys in here randomly
Could I ask you for some reviews?

As stated before I'm pretty flexible on implementation here

@shellscape

Copy link
Copy Markdown
Collaborator

Since the other two maintainers are quiet at the moment, I'll chime in to try and get this moving forward. I generally don't like to merge breaking changes unless there's a large, community-wide reason to do so. That said, I think you could add another option such as preserveOutputFilenames so that when true, the same behavior is applied without a breaking change. I would have no issue merging that for you.

@814k31

814k31 commented Sep 8, 2022

Copy link
Copy Markdown
Contributor Author

Thanks @shellscape I'll update the pr when I have time and tag you when its done

@814k31 814k31 changed the title @rollup/plugin-multi-entry - Maintain file output names when output.preserveModules = true @rollup/plugin-multi-entry: Maintain file output names when output.preserveModules = true Sep 14, 2022
@814k31

814k31 commented Sep 14, 2022

Copy link
Copy Markdown
Contributor Author

@shellscape all updated to use a new config option instead

I've also updated the pr title to hopefully match the required format, though tbh I'm not totally aware of what the required format is...

either way, look forward to hearing from you :)

@shellscape

Copy link
Copy Markdown
Collaborator

The change looks good now. We're going to be blocked from merging until #1270 lands (sorry about that). One thing to do in the mean time is to update the README with the new option.

@814k31

814k31 commented Sep 30, 2022

Copy link
Copy Markdown
Contributor Author

Sorry about the delay myself, README is updated :)

@814k31

814k31 commented Oct 14, 2022

Copy link
Copy Markdown
Contributor Author

Hey @shellscape

I noticed there were some pr conflicts so I've gone through and fixed them up
is it possible to get this merged in ASAP? It's now become a priority for my work as we are going forward with adopting rollup

Thanks!

@814k31 814k31 changed the title @rollup/plugin-multi-entry: Maintain file output names when output.preserveModules = true feat(multi-entry): add preserveModules option Oct 15, 2022
@shellscape
shellscape force-pushed the master branch 4 times, most recently from b353836 to 3038271 Compare October 21, 2022 19:01
@shellscape
shellscape merged commit 805339c into rollup:master Oct 24, 2022
@shellscape

Copy link
Copy Markdown
Collaborator

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants